Skip to content

Conversation

@andrej1307
Copy link
Owner

Спринт 12. часть 1. добавление базы данных.

new FilmGenreRowMapper());

// пополням фильмы сведениями о жанрах
for (FilmGenre filmGenre : filmsGenres) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Предложение)Данный функционал дублируется в коде. К тому же жанры не совсем подходят под данный репозиторий. Можно вынести метод по получению жанров в GenreStorage и обогащать фильмы уже в сервисе. Но очень хорошо сделано, что обращение к бд идет не в цикле, а за константное количество запросов =)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Повторяющийся код оптимизировал. Зачем нужен отдельный репозиторий для таблицы связи мне не понятно.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я не совсем про новый репозиторий. В проекте уже есть GenreDbStorage, и было бы логичнее получение жанров вынести в данный репозиторий.

*/
@Slf4j
@Service
public class FilmService {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Для сервисов также было бы хорошо использовать интерфейсы

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Добавил интерфейсы.

private final UserStorage users;

public FilmService(FilmStorage filmStorage, UserStorage users) {
public FilmService(@Qualifier("filmDbStorage") FilmStorage filmStorage,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Все InMemory реализации необходимо убрать из проекта, после этого @qualifier будет не особо нужен

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Удалил реализации InMemory.

*/
public Film updateFilm(Film updFilm) {
Integer id = updFilm.getId();
Film film = films.getFilmById(id).orElseThrow(() ->

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.orElseThrow(() лучше начинать с новой строки

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Исправлено.

public Integer addNewLike(Integer filmId, Integer userId) {
Film film = films.getFilmById(filmId).orElseThrow(() ->
new NotFoundException("Не найден фильм id=" + filmId));
users.getUserById(userId).orElseThrow(() ->

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

На 114 строчке переменная film нигде не используется в дальнейшем

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Исправлено.


@Component
@Repository("inMemoryFilmStorage")
public class InMemoryFilmStorage implements FilmStorage {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Все InMemory реализации необходимо удалить

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Удалил реализации InMemory.

import java.util.*;

@Component
@Repository("inMemoryUserStorage")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Все InMemory реализации необходимо удалить

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Удалил реализации InMemory.

-- Создаем таблицу пользователей
CREATE TABLE IF NOT EXISTS users (
id INTEGER GENERATED ALWAYS AS IDENTITY PRIMARY KEY,
email VARCHAR(255) NOT NULL,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

email можно сделать уникальным

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Исправил
....
email VARCHAR(255) UNIQUE NOT NULL,
....

return films.addNewLike(filmId, userId);
}

public Integer removeLike(Integer filmId, Integer userId) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

переменная film нигде не используется в методе, можно ее не заводить

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Исправлено.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Предложение)Упустил пару моментов в предыдущем ревью. Можно валидировать не только объекты, которые приходят в теле запроса, но и параметры, такие как count в методе findPopularFilms

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Надеюсь я правильно понял.
http://localhost:8080/films/popular?count=-1
{
"error": "400 BAD_REQUEST "Validation failure""
}

@andrej1307 andrej1307 merged commit b8ff8fa into main Mar 2, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants